feat: Crop page: increase paddings #5279
Conversation
Codecov ReportAttention: Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## develop #5279 +/- ##
==========================================
- Coverage 9.54% 8.19% -1.36%
==========================================
Files 325 356 +31
Lines 16411 17923 +1512
==========================================
- Hits 1567 1468 -99
- Misses 14844 16455 +1611 ☔ View full report in Codecov by Sentry. |
There was a problem hiding this comment.
Hi @g123k!
The most important usage of EditImageButton is not with the "Send" buttons but with the 4 "change picture" buttons.
Would you please take a screenshot of an ingredient image page with all 4 buttons? (select existing, take picture, unselect, edit)
| () => _controller.rotateLeft(), | ||
| Padding( | ||
| padding: | ||
| const EdgeInsetsDirectional.only(top: SMALL_SPACE), |
There was a problem hiding this comment.
Not sure it's a good idea to add a padding there, as it means less space for the image, that is already not that big.
There was a problem hiding this comment.
We talk about the Crop page here, not the viewer.
Are you sure it's that small?
There was a problem hiding this comment.
We do talk about the crop page.
The image is quite small on small devices, especially given that our crop tool does not zoom in.
You're right, there is no problem on big screens.
There was a problem hiding this comment.
Ok, I can only set it for bigger screens.
In the code, is there a threshold to indicate whether it's a small screen?
| Padding( | ||
| padding: const EdgeInsets.symmetric( | ||
| horizontal: VERY_SMALL_SPACE, | ||
| vertical: SMALL_SPACE, |
There was a problem hiding this comment.
Not sure it's a good idea to add a padding there, as it means less space for the image, that is already not that big.
I think there's a confusion, on the crop page, there's just the |
|
You were right, there was a regression. 
|
There was a problem hiding this comment.
Hi @g123k!
Thank you for the fix and the screenshots.
Still not convinced by the additional padding as the picture gets smaller. Which is not obvious if you take screenshots on very very large devices ;)
Just browsing the code, I've stumbled into the following enum:
enum DeviceType {
small,
smartphone,
tablet,
large,
}If you could add your paddings only for tablet and large, I'd be fine with your PR.
| }) : _centerContent = false; | ||
|
|
||
| /// Centered version of the button. | ||
| const EditImageButton.center({ | ||
| required this.iconData, | ||
| required this.label, | ||
| required this.onPressed, | ||
| this.borderWidth, | ||
| }) : _centerContent = true; |
There was a problem hiding this comment.
Wouldn't a unique constructor with this.centerContent = false be easier to maintain?
Hi everyone,
The crop page (the page displayed after the photo has been taken) has some visual issues:
Here are the changes:
